Add exceptionsToTrace configuration support in @SentinelResource annotation #543
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov Report
@@ Coverage Diff @@
## master #543 +/- ##
============================================
+ Coverage 37.63% 38.74% +1.11%
- Complexity 1100 1216 +116
============================================
Files 259 275 +16
Lines 8171 8702 +531
Branches 1113 1163 +50
============================================
+ Hits 3075 3372 +297
- Misses 4697 4900 +203
- Partials 399 430 +31
Continue to review full report at Codecov.
|
sczyh30
reviewed
Mar 6, 2019
sentinel-core/src/main/java/com/alibaba/csp/sentinel/annotation/SentinelResource.java
Outdated
Show resolved
Hide resolved
sczyh30
requested changes
Mar 13, 2019
| return false; | ||
| } | ||
| for (Class exceptionToTrace : exceptionsToTrace) { | ||
| if (ex.getClass().isAssignableFrom(exceptionToTrace)) { |
There was a problem hiding this comment.
It seems incorrect. For each exception to trace, it should be:
Suggested change
| if (ex.getClass().isAssignableFrom(exceptionToTrace)) { | |
| if (exceptionToTrace.isAssignableFrom(ex.getClass())) { |
Please be sure to add test cases and run the demo to verify it.
| @@ -59,12 +59,33 @@ public Object invokeResourceWithSentinel(ProceedingJoinPoint pjp) throws Throwab | |||
| } catch (BlockException ex) { | |||
| return handleBlockException(pjp, annotation, ex); | |||
| } catch (Throwable ex) { | |||
| Tracer.trace(ex); | |||
| if (isTrackedException(ex, annotation.exceptionsToTrace())) { | |||
There was a problem hiding this comment.
Maybe we can move the logic to AbstractSentinelAspectSupport class to support extension for sub-classes. E.g.:
protected void traceException(Throwable ex, SentinelResource annotation)
sczyh30
changed the title
sczyh30
approved these changes
Mar 25, 2019
|
Thanks for contributing. I'll help to update the test cases and refactor code later. Please note that every new feature or enhancement must carry corresponding test cases. |
|
Improved in 61c5250 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Describe what this PR does / why we need it
In some scenarios，this resource will be degraded when it throws a specific exception.
So the configuration of exception class that should be traced (blocked), is needed.
Does this pull request fix one issue?
Fixes #540
Describe how you did it
Add the configuration of exception classes that would be traced.
Describe how to verify it
None
Special notes for reviews
None